Skip to content

Convert priority 3 MethodDescCallSite calls to UnmanagedCallersOnly#124854

Open
steveisok wants to merge 16 commits intodotnet:mainfrom
steveisok:uco-excep-p3
Open

Convert priority 3 MethodDescCallSite calls to UnmanagedCallersOnly#124854
steveisok wants to merge 16 commits intodotnet:mainfrom
steveisok:uco-excep-p3

Conversation

@steveisok
Copy link
Member

First set of priority 3 conversions from #123864 — replace MethodDescCallSite/CallDescrWorker calls with UnmanagedCallersOnly reverse P/Invoke calls.

Converted call sites

File Call site Method
appdomain.cpp AppDomain::RaiseExitProcessEvent ON_PROCESS_EXIT
appdomain.cpp AppDomain::OnUnhandledException ON_UNHANDLED_EXCEPTION
assembly.cpp RunManagedStartup MANAGED_STARTUP
assembly.cpp RunMainInternal (TARGET_BROWSER) HANDLE_ASYNC_ENTRYPOINT
invokeutil.cpp InvokeUtil::CreateClassLoadExcept ReflectionTypeLoadException ctor
invokeutil.cpp InvokeUtil::CreateTargetExcept TargetInvocationException ctor

Remaining priority 3 items (follow ups)

  • Main entry point in RunMainInternal (dynamic MethodDesc)
  • CustomAttribute_CreateCustomAttributeInstance (dynamic ctor, already a QCall)
  • CorHost2::ExecuteAssembly (dynamic method lookup)
  • FuncEvalWrapper/DoNormalFuncEval (debugger func-eval)

Pattern

Follows the same conversion pattern as #124834 (priority 2). Each conversion adds an [UnmanagedCallersOnly] managed wrapper with an Exception* out-parameter and replaces the native MethodDescCallSite + ARG_SLOT with
UnmanagedCallersOnlyCaller::InvokeThrowing.

Replace MethodDescCallSite/CallDescrWorker calls with more efficient
UnmanagedCallersOnly reverse P/Invoke calls for priority 3 items from
dotnet#123864.

Converted call sites:
- AppDomain::RaiseExitProcessEvent (ON_PROCESS_EXIT)
- AppDomain::OnUnhandledException (ON_UNHANDLED_EXCEPTION)
- RunManagedStartup (MANAGED_STARTUP)
- RunMainInternal async helpers (HANDLE_ASYNC_ENTRYPOINT, TARGET_BROWSER)
- InvokeUtil::CreateClassLoadExcept (ReflectionTypeLoadException ctor)
- InvokeUtil::CreateTargetExcept (TargetInvocationException ctor)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@dotnet-policy-service
Copy link
Contributor

Tagging subscribers to this area: @agocke
See info in area-owners.md if you want to be subscribed.

…-param and managed GetDefinedTypes() throws ReflectionTypeLoadException. Deleted reateClassLoadExcept + UCO wrapper + metasig as a result.

Adjusted OnUnhandledException in AppContext.CoreCLR.cs to use a bare catch based on feedback.
Copilot AI review requested due to automatic review settings February 25, 2026 16:49
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR converts priority 3 MethodDescCallSite/CallDescrWorker calls to more efficient UnmanagedCallersOnly reverse P/Invoke calls, following the established pattern from PR #124834. The conversions target application entry points and process lifecycle events in CoreCLR.

Changes:

  • Adds [UnmanagedCallersOnly] wrappers in managed code with Exception* out-parameters for exception propagation
  • Replaces native MethodDescCallSite calls with UnmanagedCallersOnlyCaller::InvokeThrowing template class
  • Refactors ReflectionTypeLoadException creation from native to managed code via QCall pattern

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncHelpers.Browser.cs Adds UCO wrapper for HandleAsyncEntryPoint to handle async Main entry points on Browser target
src/coreclr/System.Private.CoreLib/src/System/StartupHookProvider.CoreCLR.cs Adds UCO wrapper for ManagedStartup with exception handling
src/coreclr/System.Private.CoreLib/src/System/Exception.CoreCLR.cs Adds UCO wrapper for CreateTargetInvocationException
src/coreclr/System.Private.CoreLib/src/System/AppContext.CoreCLR.cs New file with UCO wrappers for OnProcessExit and OnUnhandledException
src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeModule.cs Updates GetTypes QCall to return exceptions array for managed-side exception construction
src/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj Adds new AppContext.CoreCLR.cs file to build
src/coreclr/vm/metasig.h Adds new metasig definitions for UCO method signatures
src/coreclr/vm/corelib.h Updates DEFINE_METHOD entries to reference new UCO signatures
src/coreclr/vm/appdomain.cpp Replaces MethodDescCallSite with UnmanagedCallersOnlyCaller for process lifecycle events
src/coreclr/vm/assembly.cpp Replaces MethodDescCallSite with UnmanagedCallersOnlyCaller for startup hooks and async entry points
src/coreclr/vm/invokeutil.h Removes obsolete CreateClassLoadExcept declaration
src/coreclr/vm/invokeutil.cpp Removes CreateClassLoadExcept implementation; converts CreateTargetExcept to use UCO pattern
src/coreclr/vm/commodule.h Adds retExceptions parameter to RuntimeModule_GetTypes QCall signature
src/coreclr/vm/commodule.cpp Returns exceptions array via out-parameter instead of throwing from native code
src/coreclr/vm/exceptionhandlingqcalls.h Formatting fix - adds blank line at end of file

Copilot AI review requested due to automatic review settings February 25, 2026 18:50
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/coreclr/vm/commodule.cpp:766

  • RuntimeModule_GetTypes no longer throws inside the QCall when type loading fails, but the loop still doesn’t advance curPos on failure. With retExceptions.Set(...) replacing the throw, execution now continues and will hit _ASSERTE(curPos == dwNumTypeDefs) when any type load fails. Adjust the loop to keep curPos in sync with dwNumTypeDefs (e.g., advance and leave a null slot for failed loads, or remove/replace the assertion and ensure the returned Types array matches the intended semantics).
    // Return exceptions to managed side for throwing
    if (cXcept > 0) {

        gc.xceptRet = (PTRARRAYREF) AllocateObjectArray(cXcept,g_pExceptionClass);
        for (DWORD i=0;i<cXcept;i++) {
            gc.xceptRet->SetAt(i, gc.xcept->GetAt(i));
        }
        retExceptions.Set(gc.xceptRet);
    }

    // We should have filled the array exactly.
    _ASSERTE(curPos == dwNumTypeDefs);

@jkotas
Copy link
Member

jkotas commented Feb 27, 2026

Test failure

ASSERT FAILED
	Expression: curPos == dwNumTypeDefs
	Location:   /__w/1/s/src/coreclr/vm/commodule.cpp:765
	Function:   RuntimeModule_GetTypes
	Process:    84956

@jkotas
Copy link
Member

jkotas commented Feb 27, 2026

The wasm temporary callhelpers needs to be regenerated as part of these PRs: https://github.com/dotnet/runtime/blob/main/docs/workflow/wasm-documentation.md#running-coreclr-callhelpers-generator

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.

Copilot AI review requested due to automatic review settings February 27, 2026 15:36
{"libSystem.Globalization.Native", s_libSystem_Globalization_Native, 33},
{"libSystem.IO.Compression.Native", s_libSystem_IO_Compression_Native, 8},
{"libSystem.Native", s_libSystem_Native, 97},
{"libSystem.Native", s_libSystem_Native, 88},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what happened that this number is going down. This PR should not be changing this file.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was curious of that too. Problem with the script or am I the first to run it on linux?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@radekdoulik @AaronRobinsonMSFT Have you seen this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. There is some reliability issue with the generator. I've no clue why it goes down. I saw it on Windows and then moved to macOS because I was suspicious of it. I was going to try and find some time to investigate, unclear if I'll find that time. Running on Windows has been painful from a consistency/reliability perspective.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to coordinate with @radekdoulik and see what needs to happen. I'll run the regeneration once I have a better idea.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.

Copilot AI review requested due to automatic review settings February 27, 2026 15:45
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.

steveisok and others added 2 commits February 27, 2026 11:16
The generated output included incorrect removals of System.Net.Primitives
networking functions and accumulated drift from main. Will regenerate
properly in a follow-up with a complete browser build.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
#include <string.h>

#ifdef TARGET_BROWSER
extern "C" void SystemJS_MarkAsyncMain();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intentional change?

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants